-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move "composer/installers" package to require-dev. #1513
Conversation
Size Change: 0 B Total Size: 201 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, @mreishus. 👋🏼 I am probably not the most suitable person to judge the change proposed in this PR so will let it for others.
Thanks for the review request anyways!
@Automattic/somewherewarm Could you help with a review for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍 I did a quick test on a E-commerce plan site and all seems to be working fine. Thanks for the PR. @mreishus
Hey @obenland - SomewhereWarm hasn't worked with Calypso Bridge for over six months. Someone that is using Calypso Bridge on a daily basis, should be better suited to test this on out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing, @chihsuan!
@mreishus Apologies to have missed this earlier. I've also tested this change in a business and a ecommerce plan site. I'm unsure how to fully test the change, but what I did was:
- In a fresh clone, run
npm install
,composer install
, andnpm run build
and observed no related error message - Upload the entire directory to
wp-content/mu-plugins/wpcomsh/vendor/automattic/wc-calypso-bridge
of the site - Tested wp-admin and frontend and observed no error
- Looked at
wp php-errors
and observed no error - Looked at
wp-content/debug.log
and observed no error
I've also updated the changelog in 23f91b7.
LGTM, feel free to merge. Let me know if you need help with deploying
Thanks for the review! Created deploy PR #1526 |
Changes proposed in this Pull Request:
I found that wpcomsh was shipping with a large number of installer files in vendor and in the classmap:
These don't look relevant to anything we need to do. Tracing it down, this dependency was added here when adding linting commands to the repo: #90
It might have also been added here instead: a9a44f4
As far as I can tell,
composer/installers
lets you install plugins to places other thanvendor/
, but I don't see it being used here. @anomiex also ran into some issues from it being included here: Automattic/jetpack#37737 (comment)I think that means this is only needed for development (if at all), and we don't need to be including this when shipping the plugin. I searched for instances of "installer" and don't see it being used anywhere.
How to test the changes in this Pull Request:
Other information:
FOR PR REVIEWER ONLY: